-
-
Notifications
You must be signed in to change notification settings - Fork 11
Combine const and enum errors into a single message
#155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Combine const and enum errors into a single message
#155
Conversation
7646a94 to
ac73647
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach isn't going to work. Try,
{
"allOf": [
{ "const": "a" },
{ "const": "b" }
]
}ac73647 to
3b4eaf2
Compare
Hi @jdesrosiers, thanks for pointing out the issue! I understood the problem: Issue: with a schema like above one ->
const passed = normalizedErrors[...][schemaLocation] === true;
if (!passed) hasFailure = true;
// always collect
constraints.push({ allowedValues: [...], schemaLocation });now:
Tests Added:
All tests pass. Is this approach good, or does it need further improvements? |
jdesrosiers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should work. Just a couple small things I'd like to see.
src/error-handlers/constAndEnum.js
Outdated
| const sorted = [...constraints].sort((a, b) => | ||
| a.allowedValues.length - b.allowedValues.length | ||
| ); | ||
| const mostConstraining = sorted[0]; | ||
|
|
||
| let intersection = mostConstraining.allowedValues; | ||
| for (let i = 1; i < sorted.length; i++) { | ||
| intersection = intersection.filter((/** @type {Json} */ val) => | ||
| sorted[i].allowedValues.some((/** @type {Json} */ other) => jsonEqual(val, other)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this could be avoided if you used a Set and Set operations to determine the intersection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored to use Set operations for intersection, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly the same code. All you did was wrap your array in a Set and then convert it back to an array. Have look at Set.intersection and try again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I looked into Set.intersection() but didn't use it due to compatibility concerns, it requires Node 22+, Chrome 122+, Safari 17+
The manual approach ensures broader compatibility. should I switch to Set.intersection(), or is the manual approach preferred for this project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Use Set.intersection.
3b4eaf2 to
e7b9dec
Compare
e7b9dec to
219a62a
Compare
Description
When multiple
constorenumconstraints fail (e.g., viaallOf), produce a single message with the most restrictive constraint instead of multiple redundant messages.enummessageChecklist